Added tests for unsupported braces in custom format specifiers. #35820

mikernet · 2019-03-06T17:09:32Z

Test modifications for issue #35167 with related coreclr PR for the associated StringBuilder modifications here:

dotnet/coreclr#23062

CustomFormatter was modified to track the last format string passed in.

dnfclas · 2019-03-06T17:09:45Z

All CLA requirements met.

karelz · 2019-04-01T20:43:36Z

@bartonjs can you please review the change?
@mikernet did you look at the test failures?

mikernet · 2019-04-01T20:48:00Z

@mikernet did you look at the test failures?

@karelz No I didn't, because it depends on the changes in the referenced CoreCLR pull request to succeed so you guys gotta do your inner loop testing process.

mikernet · 2019-04-01T20:50:56Z

@karelz I don't really know how the back and forth process works - the other PR was accepted, so how do I get it to rerun the tests against that?

karelz · 2019-04-01T21:28:54Z

inner loop testing = what anyone can run locally :)
Changes across CoreCLR and CoreFX are usually a bit more complicated and involved. We should wait for CoreCLR PR to be merged first. Then we can merge this one (marking it blocked).
That said, the dev is expected to run the tests on the CoreCLR changes locally prior to merging the PR in CoreCLR. Did you do that? (there are some advanced docs in our repo about it)

mikernet · 2019-04-01T22:12:42Z

Yes I ran them locally. The issue here is they are failing in CI.

karelz · 2019-04-01T22:15:37Z

@mikernet and they will be until CoreCLR PR is merged and ingested in CoreFX :)

danmoseley · 2019-04-01T22:40:57Z

I guess then it's waiting on #36480 (or its successor)

ViktorHofer · 2019-05-18T17:34:03Z

@mikernet seems you aren't blocked anymore, do you need any help to get this ready?

ViktorHofer · 2019-05-18T17:35:51Z

/azp run corefx-ci

azure-pipelines · 2019-05-18T17:36:09Z

Azure Pipelines successfully started running 1 pipeline(s).

mikernet · 2019-05-18T18:05:29Z

@ViktorHofer I'm not really sure what you need me to do here. The tests compile and run successfully on my system with the changes in the other referenced PR.

ViktorHofer · 2019-05-18T18:08:16Z

The test is failing on OSX:

Assert.Equal() Failure\n ↓ (pos 0)\nExpected: 0}\nActual: }\n ↑ (pos 0)

   at System.Text.Tests.StringBuilderTests.AppendFormat_NoEscapedBracesInCustomFormatSpecifier() in /Users/vsts/agent/2.150.3/work/1/s/src/System.Runtime/tests/System/Text/StringBuilderTests.cs:line 933

mikernet · 2019-05-18T18:16:30Z

I don't know enough about how this build system works to figure out why that would be. If OSX is running the latest .NET Core string builder code that I modified in the referenced PR then there's no reason for it to fail - it's running the same code as the windows and linux builds. If the OSX test runs some other version of .NET / .NET Core then that test will need to be excluded from running on OSX.

There's an exclusion on that test for .NET Framework since that won't get the StringBuilder changes, so the test is marked with [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]. OSX should be running the latest .NET Core changes though should it not? Based on the test results it's fairly clear that it isn't though.

mikernet · 2019-05-18T18:20:02Z

Looking at the new results rolling in something odd is clearly going on and I don't really know why the new StringBuilder code doesn't seem to be running everywhere.

mikernet · 2019-05-18T18:26:21Z

Has the referenced PR even been merged yet? It is showing as open so I'm not sure what you want me to do.

mikernet · 2019-05-18T18:40:07Z

The stranger thing to me is that Linux musl_x64_Debug is passing but Linux musl_x64_Release is failing. One of them is apparently running skipping the [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] test and the other isn't??

Why anything that isn't NETFX is passing I have no idea. That additional test should be failing anything not running the new StringBuilder code. This is all very confusing.

… specifiers (#23062) * Modified format logic so that braces are not allowed in custom format specifiers. * Disabled failing test temporarily and fixed code style issue. PR needed to pass test: dotnet/corefx#35820 * Slight changes to logic to better align the diff. * Fixed counting error. * Fix nullable * Move exclusion near related test * Add skip to new issue file * Delete CoreFX.issues.json

… specifiers (dotnet#23062) * Modified format logic so that braces are not allowed in custom format specifiers. * Disabled failing test temporarily and fixed code style issue. PR needed to pass test: dotnet#35820 * Slight changes to logic to better align the diff. * Fixed counting error. * Fix nullable * Move exclusion near related test * Add skip to new issue file * Delete CoreFX.issues.json Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

… specifiers (#23062) * Modified format logic so that braces are not allowed in custom format specifiers. * Disabled failing test temporarily and fixed code style issue. PR needed to pass test: dotnet/corefx#35820 * Slight changes to logic to better align the diff. * Fixed counting error. * Fix nullable * Move exclusion near related test * Add skip to new issue file * Delete CoreFX.issues.json Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

… specifiers (#23062) * Modified format logic so that braces are not allowed in custom format specifiers. * Disabled failing test temporarily and fixed code style issue. PR needed to pass test: #35820 * Slight changes to logic to better align the diff. * Fixed counting error. * Fix nullable * Move exclusion near related test * Add skip to new issue file * Delete CoreFX.issues.json Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

… specifiers (#23062) * Modified format logic so that braces are not allowed in custom format specifiers. * Disabled failing test temporarily and fixed code style issue. PR needed to pass test: dotnet/corefx#35820 * Slight changes to logic to better align the diff. * Fixed counting error. * Fix nullable * Move exclusion near related test * Add skip to new issue file * Delete CoreFX.issues.json Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

JeremyKuhne · 2019-05-22T21:18:22Z

Has the referenced PR even been merged yet?

I just finally got it merged yesterday. Hopefully the next CoreCLR integration will have the change- I'll keep an eye out for it.

stephentoub · 2019-05-28T00:19:37Z

Cherry-picked into #37953. Thanks.

… specifiers (dotnet/coreclr#23062) * Modified format logic so that braces are not allowed in custom format specifiers. * Disabled failing test temporarily and fixed code style issue. PR needed to pass test: dotnet/corefx#35820 * Slight changes to logic to better align the diff. * Fixed counting error. * Fix nullable * Move exclusion near related test * Add skip to new issue file * Delete CoreFX.issues.json Commit migrated from dotnet/coreclr@5d8fea0

Added tests for unsupported braces in custom format specifiers.

6b69f87

CustomFormatter was modified to track the last format string passed in.

mikernet mentioned this pull request Mar 6, 2019

Modified format logic so that braces are not allowed in custom format specifiers dotnet/coreclr#23062

Merged

mikernet added 3 commits March 6, 2019 12:31

Fixed call to String.Contains overload that doesn't exist on netfx.

6f7353e

Simplified format specifier brace escaping test.

b87dc99

Minor style and spacing change.

a9f0640

danmoseley requested a review from JeremyKuhne March 12, 2019 00:48

karelz added the area-System.Runtime label Mar 18, 2019

karelz assigned mikernet and bartonjs Mar 18, 2019

karelz requested a review from bartonjs April 1, 2019 20:43

karelz added the blocked Issue/PR is blocked on something - see comments label Apr 1, 2019

karelz added this to the 3.0 milestone Apr 1, 2019

dotnet deleted a comment from azure-pipelines bot May 18, 2019

stephentoub closed this May 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for unsupported braces in custom format specifiers. #35820

Added tests for unsupported braces in custom format specifiers. #35820

mikernet commented Mar 6, 2019

dnfclas commented Mar 6, 2019 •

edited

Loading

karelz commented Apr 1, 2019

mikernet commented Apr 1, 2019 •

edited

Loading

mikernet commented Apr 1, 2019

karelz commented Apr 1, 2019

mikernet commented Apr 1, 2019

karelz commented Apr 1, 2019

danmoseley commented Apr 1, 2019

ViktorHofer commented May 18, 2019

ViktorHofer commented May 18, 2019

azure-pipelines bot commented May 18, 2019

mikernet commented May 18, 2019

ViktorHofer commented May 18, 2019

mikernet commented May 18, 2019 •

edited

Loading

mikernet commented May 18, 2019

mikernet commented May 18, 2019 •

edited

Loading

mikernet commented May 18, 2019 •

edited

Loading

JeremyKuhne commented May 22, 2019

stephentoub commented May 28, 2019

Added tests for unsupported braces in custom format specifiers. #35820

Added tests for unsupported braces in custom format specifiers. #35820

Conversation

mikernet commented Mar 6, 2019

dnfclas commented Mar 6, 2019 • edited Loading

karelz commented Apr 1, 2019

mikernet commented Apr 1, 2019 • edited Loading

mikernet commented Apr 1, 2019

karelz commented Apr 1, 2019

mikernet commented Apr 1, 2019

karelz commented Apr 1, 2019

danmoseley commented Apr 1, 2019

ViktorHofer commented May 18, 2019

ViktorHofer commented May 18, 2019

azure-pipelines bot commented May 18, 2019

mikernet commented May 18, 2019

ViktorHofer commented May 18, 2019

mikernet commented May 18, 2019 • edited Loading

mikernet commented May 18, 2019

mikernet commented May 18, 2019 • edited Loading

mikernet commented May 18, 2019 • edited Loading

JeremyKuhne commented May 22, 2019

stephentoub commented May 28, 2019

dnfclas commented Mar 6, 2019 •

edited

Loading

mikernet commented Apr 1, 2019 •

edited

Loading

mikernet commented May 18, 2019 •

edited

Loading

mikernet commented May 18, 2019 •

edited

Loading

mikernet commented May 18, 2019 •

edited

Loading